db.get mock missing "publishers:org" — positive test never exercises new code

With ownerPublisherId: "publishers:org" and personalPublisher._id = "publishers:personal", the handler calls requirePublisherRole(ctx, { publisherId: "publishers:org", ... }) at skills.ts:5788. That function calls ctx.db.get("publishers:org"), which returns null here, so isPublisherActive(null) → false → throws ConvexError("Publisher not found"). patch is never called and the expect(patch).toHaveBeenCalledWith(...) assertion fails.

Add the missing publisher to the get mock:

The same gap exists in the third test ("updates slug aliases", line 1501–1508) — add the identical "publishers:org" branch there as well.

This is a comment left during a code review.
Path: convex/skills.rateLimit.test.ts
Line: 1364-1376

Comment:
**`db.get` mock missing `"publishers:org"` — positive test never exercises new code**

With `ownerPublisherId: "publishers:org"` and `personalPublisher._id = "publishers:personal"`, the handler calls `requirePublisherRole(ctx, { publisherId: "publishers:org", ... })` at `skills.ts:5788`. That function calls `ctx.db.get("publishers:org")`, which returns `null` here, so `isPublisherActive(null)` → false → throws `ConvexError("Publisher not found")`. `patch` is never called and the `expect(patch).toHaveBeenCalledWith(...)` assertion fails.

Add the missing publisher to the `get` mock:

```suggestion
      get: vi.fn(async (id: string) => {
        if (id === "users:owner") {
          return {
            _id: "users:owner",
            handle: "pushmatrix",
            deletedAt: undefined,
            deactivatedAt: undefined,
          };
        }
        if (id === "publishers:personal") {
          return { _id: "publishers:personal", kind: "user", linkedUserId: "users:owner" };
        }
        if (id === "publishers:org") {
          return { _id: "publishers:org", kind: "org", deletedAt: undefined, deactivatedAt: undefined };
        }
        return null;
      }),
```

The same gap exists in the third test ("updates slug aliases", line 1501–1508) — add the identical `"publishers:org"` branch there as well.

How can I resolve this? If you propose a fix, please make it concise.

Wrong error asserted — test fails for the wrong reason

The test expects rejects.toThrow(/slug is already taken/i), but execution never reaches the slug-conflict check. Because ownerPublisherId: "publishers:strangerOrg" ≠ personalPublisher._id ("publishers:personal"), requirePublisherRole is called. ctx.db.get("publishers:strangerOrg") returns null (not in the mock), so the function throws ConvexError("Publisher not found") — not "slug is already taken".

For this test to validate that a non-owner is blocked at the slug level, the mock needs "publishers:strangerOrg" to resolve as a valid publisher, and the stranger must appear as a member. Without those, the test only confirms that publishing to a non-existent publisher is blocked, which is already covered elsewhere.

This is a comment left during a code review.
Path: convex/skills.rateLimit.test.ts
Line: 1440-1495

Comment:
**Wrong error asserted — test fails for the wrong reason**

The test expects `rejects.toThrow(/slug is already taken/i)`, but execution never reaches the slug-conflict check. Because `ownerPublisherId: "publishers:strangerOrg"` ≠ `personalPublisher._id` (`"publishers:personal"`), `requirePublisherRole` is called. `ctx.db.get("publishers:strangerOrg")` returns `null` (not in the mock), so the function throws `ConvexError("Publisher not found")` — not "slug is already taken".

For this test to validate that a non-owner is blocked at the slug level, the mock needs `"publishers:strangerOrg"` to resolve as a valid publisher, and the stranger must appear as a member. Without those, the test only confirms that publishing to a non-existent publisher is blocked, which is already covered elsewhere.

How can I resolve this? If you propose a fix, please make it concise.

Mock target publisher before asserting move-publisher behavior

These new tests pass ownerPublisherId values like publishers:org, but the db.get mock only returns publishers:personal and otherwise returns null. In insertVersion, that causes requirePublisherRole to fail with Publisher not found before the owner/non-owner slug logic runs, so the tests do not actually exercise the behavior they claim to validate and are likely to fail for the wrong reason.

Useful? React with 👍 / 👎.

Preserve owner publisher when target publisher is omitted

This branch now always patches ownerPublisherId when the caller owns the skill and the resolved publisher differs, but ownerPublisherId is resolved to the caller’s personal publisher when omitted (insertVersion sets args.ownerPublisherId ?? personalPublisher._id). Legacy publish paths such as convex/httpApi.ts (publishVersionForUser(ctx, userId, args)) and convex/httpApiV1/skillsV1.ts also call publish without an owner publisher, so publishing an org-owned skill through those endpoints will silently transfer ownership (and alias ownership) from the org to personal. This is a behavior regression from “reject with slug taken” to “implicit ownership move” without explicit intent.

Useful? React with 👍 / 👎.

Re-validate org access before preserving ownerPublisherId

In the legacy-path branch (skill.ownerUserId === userId with no explicit ownerPublisherId), this code rewrites ownerPublisherId to skill.ownerPublisherId after the only authorization check has already run against the caller’s personal publisher. That means legacy publish endpoints that call publishVersionForUser without an ownerPublisherId (e.g. convex/httpApi.ts and convex/httpApiV1/skillsV1.ts) now allow publishing under an org-owned skill even when the caller no longer has org membership (or the org publisher is inactive), because no requirePublisherRole check is performed for the preserved org publisher.

Useful? React with 👍 / 👎.

Require explicit move signal before changing owner publisher

This branch assumes args.ownerPublisherId means the caller explicitly asked to transfer ownership, but the normal web publish path always supplies an ownerPublisherId (via publishVersion target resolution) and the publish form defaults ownerHandle to the personal publisher for updates. In that flow, republishing an org-owned skill can now hit this branch and patch ownerPublisherId to personal even when the user only intended to publish a new version, creating an unintended ownership transfer regression from the previous slug-taken guard.

Useful? React with 👍 / 👎.

Require explicit transfer signal before patching ownerPublisherId

Using Boolean(args.ownerPublisherId) as the transfer signal is unsafe because this field is auto-populated in normal publish flows rather than being a true user-intent flag. publishVersion always passes target.publisherId, and the web update flow auto-fills owner handle to the personal publisher (src/routes/publish-skill.tsx:204-210, reached from src/routes/dashboard.tsx:328), so republishing an org-owned skill can hit this branch and silently move ownership to personal via ctx.db.patch(skill._id, ...). Fresh evidence is that the new branch now performs ownership moves on that defaulted path, making this an introduced behavior regression rather than a theoretical caller misuse.

Useful? React with 👍 / 👎.